Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Current library's bib file is removed from the unlinked file search results #9755

Merged
merged 6 commits into from
Apr 13, 2023

Conversation

NS404
Copy link
Contributor

@NS404 NS404 commented Apr 11, 2023

Fixes #9735

Compulsory checks

@NS404 NS404 changed the title Searching for unlinked files includes the current library's bib file in the results Current library's bib file is removed from the unlinked file search results Apr 11, 2023
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congratulations on your first contribution! 💯
Can you also please add a Test case for this? It should be pretty straightforward to add it in UnlinkedFilesCrawlerTest
Just create a new BibDatabasContext and set the path

@@ -37,6 +39,7 @@ public DatabaseFileLookup(BibDatabaseContext databaseContext, FilePreferences fi
for (BibEntry entry : databaseContext.getDatabase().getEntries()) {
fileCache.addAll(parseFileField(entry));
}
this.pathOfDatabase = databaseContext.getDatabasePath().orElseThrow(() -> new NullPointerException("Database null"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather say, return an empty string in case no database path is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the opportunity! : )
I thought that an instance of DataBaseLookup instance should not exist if a database path is not present. Why not throw an exception?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be an edge case for a shared database, that is synced with an external SQL database but not necessarily stored on disk but only kept/edited in memory

Copy link
Contributor Author

@NS404 NS404 Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 12, 2023
Comment on lines +73 to +75
public Path getPathOfDatabase() {
return pathOfDatabase;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most clean code would be to use Optional<Path> here. However, the only place using this would be more complex. @Siedlerchr Any oppinion here?

If this is kept as is, the JavaDoc comment should be put here

@returns "" if the Path does not exist

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep as is

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just wrong. A Path object can never be an empty string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return null then? Then, the calling code would be "clean". Think this method is never reached if the database is not saved.

Siedlerchr
Siedlerchr previously approved these changes Apr 13, 2023
@Siedlerchr Siedlerchr merged commit 81f2688 into JabRef:main Apr 13, 2023
@Siedlerchr
Copy link
Member

Thanks again for your contribution! Looking forward for seeing more from you :)
And I hope you didn't mind the "nitpicks" (that is a good sign, that the rest of the code is already good :) )

@NS404
Copy link
Contributor Author

NS404 commented Apr 13, 2023

Not at all, I enjoyed it! Thank you :)

@NS404 NS404 deleted the fix-for-issue-9735 branch April 13, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search for unlinked files should not include current bib file
4 participants